Patches from Photon OS #1428
Patches from Photon OS #1428
Conversation
In commit [1], kpatch added support for function padding, and CONFIG_CFI_CLANG, which hardcoded a value of 16 for the prefix size. In some cases, the padding around __cfi prefixed functions can vary. For example, in Photon OS 5.0, the __cfi prefix size is modified in a patch for the gcc RAP plugin [2]. Since we have read the prefix size anyways, we can use it instead of hardcoding. Ref: 1. dynup@3e54c63 2. https://github.com/vmware/photon/blob/5.0/SPECS/linux/secure/gcc-rap-plugin-with-kcfi.patch Signed-off-by: Brennan Lamoreaux <[email protected]>
|
Hi @bhllamoreaux, thanks for posting. (1) The symbol prefix change looks good. (2) Hmm, the old kpatch-patch-hook.c file was to support legacy, pre-livepatch-support kernels. I suppose the only valid use case today would be a modern kernel with !CONFIG_LIVEPATCH ... for which I don't think we really want/need to support in kpatch-build anymore. Let me think a bit more on this as maybe we just finally rip out that old code and simplify this problem. |
|
Hi @bhllamoreaux - sorry for the delay and happy new year. I'm curious how your setup is incorporating the $ kpatch-build -d -s ~/linux test/integration/linux-6.2.0/syscall.patch
$ cd ~/.kpatch/tmp/patch
$ source ~/.kpatch/tmp/kpatch-build.env
$ rm -f patch-hook.o livepatch-syscall.ko
$ KCFLAGS='-Wbad-function-cast' makebut the compile blows up with many non-matching casts from a slew of stock kernel header files. This is with a v6.12 kernel tree and gcc 11.5. Interestingly it doesn't complain about the callback casts, so maybe I'm doing this wrong. |
|
Hi, happy new year! Sorry for the delayed response. Interestingly, we never saw this issue on Photon 5 previous to the past few months, and the kpatch version hasn't changed (still latest release of v0.9.8). I'm really not sure what changed to cause it... But it is reproducible every time. And definitely I'm not seeing any other cast errors besides this one. Maybe it is something particular to our current Photon 5 release which isn't there in other distros? It's reproducible without any changes to the standard Photon environment. I can try to dig deeper if I get some more free time. Here's some steps which should reproduce it on Photon: This is the error output I see: |
|
Hi @bhllamoreaux , thanks for the additional detail. I can't build that kernel locally, as it throws this error: which I suspect is due to the fact that With your version of gcc, does this minimal source file reproduce the error? /* build with: -Wbad-function-cast -c test.c -o test */
/* include/linux/livepatch.h */
struct klp_object;
struct klp_callbacks {
int (*pre_patch)(struct klp_object *obj);
void (*post_patch)(struct klp_object *obj);
void (*pre_unpatch)(struct klp_object *obj);
void (*post_unpatch)(struct klp_object *obj);
/* ... */
};
struct klp_object {
/* ... */
struct klp_callbacks callbacks;
/* ... */
};
/* kmod/patch/kpatch-patch.h */
struct kpatch_post_unpatch_callback {
void (*callback)(void *obj);
/* ... */
};
/* kmod/patch/livepatch-patch-hook.c */
struct patch_object {
/* ... */
struct klp_callbacks callbacks;
/* ... */
};
/* kmod/patch/livepatch-patch-hook.c :: add_callbacks_to_patch_objects() */
void add_callbacks_to_patch_objects()
{
struct kpatch_post_unpatch_callback *p_post_unpatch_callback;
struct patch_object *object;
object->callbacks.post_unpatch = (void (*)(struct klp_object *))
p_post_unpatch_callback->callback;
}Unfortunately I can't repro those casting errors with |
|
Hi @joe-lawrence, Oh, interesting. Maybe you'd need to try to build Photon on Photon... To be honest, I haven't ever tried it on other distros. Sorry about that. I tried with that test file with my gcc, but unfortunately I'm not able to reproduce it that way either. If I run kpatch with --debug and keep the scratch files, I can also reproduce it like this (same build as the details I provided above): And, if I replace livepatch-patch-hook.c with your test.c, it also reproduces: So, maybe it's not |
|
I think it's because this step is building like a Linux module, which adds a bunch of Linux gcc flags. This is the command I see getting executed for me: Is it different for you? I tried your test.c with |
|
Yeah I was about to suggest running the build with V=1 to see what the kernel build was adding to the gcc line. Here is mine: I put yours and mine build lines into separate files, substituted spaces with linebreaks, sorted each file, and then displayed the lines unique to yours and get: Keep in mind there are a few options where our paths were slightly different, affecting the I tried compiling test.c with all of your -W options, and I see a few different complains, but not the casting ones. I wonder if you tried building with those |
|
Aha, looks like we've found the culprit. The build error is being caused by this option: |
Ahah! Thanks for hunting that down. I'm surprised the kernel itself doesn't hit any of the same casting problems, but perhaps those have all been fixed by now. Anyway, although this does at first seem like a straightforward thing to fix, I think I'd like to defer on touching this at the moment. There is some annoying complexity imposed by the current code which operates under a few different contexts: a) modern kernels without CONFIG_LIVEPATCH, b) kernels with CONFIG_LIVEPATCH but not callbacks, c) kernels with CONFIG_LIVEPATCH and callbacks, d) kernels with CONFIG_LIVEPATCH but people want the legacy kpatch.ko helper module for some awful reason, etc. A few of those header files / definitions are shared between kernel and user programs.. arg. As I mentioned at the top, this gets a lot simpler when we finally pull the plug on kpatch.ko (perhaps we bump kernel requirements to force callback support at the same time.) Would it be a burden for Photon to carry the second patch at the distro level in the interim? (The first patch still looks good for upstream.) |
|
Gotcha, makes sense. That's totally fine, I can drop the second patch from the PR and we'll just keep it downstream in Photon for now. |
bhllamoreaux
force-pushed
the
photon-patches
branch
from
5f12b31 to
846db25
Compare
These two patches are logically separate but are based on recent fixes for bugs I encountered recently in Photon OS. I think they can also be applicable to upstream kpatch.
The changes are: